Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Entity): Do not add Array.prototype properties to store #782

Merged
merged 1 commit into from
Feb 9, 2018

Conversation

livthomas
Copy link
Contributor

I have tried to fix issue #781 and verified the problem is really no longer there by building my application (where I can always reproduce the problem) with a version of @ngrx/entity containing these changes.

@coveralls
Copy link

coveralls commented Feb 4, 2018

Coverage Status

Coverage increased (+0.2%) to 93.016% when pulling f311f39 on livthomas:entity-null into ecf1ebf on ngrx:master.

@MikeRyanDev
Copy link
Member

It sounds like this is hard to reproduce. Any chance you can isolate it into a unit test that fails on current version but passes with this fix?

@livthomas
Copy link
Contributor Author

I simply cannot reproduce it in the unit tests. But I think I have found the root cause. My project uses core-js library which adds some additional Array.prototype methods and the for..in loop in @ngrx/entity code then iterates over them (as well as over the indices).

In general, iterating over an array with for..in loop is a very bad idea and should be avoided. See the relevant sections of Speaking JavaScript book or MDN documentation.

@livthomas livthomas changed the title fix(Entity): Do not store null entity keys fix(Entity): Do not add Array.prototype properties to store Feb 9, 2018
@livthomas
Copy link
Contributor Author

@MikeRyanDev, I have added a reproducer here as you suggested.

@MikeRyanDev MikeRyanDev merged commit d537758 into ngrx:master Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants